Skip to content

feat(sdk-core): extract isMpcV2Keycard and signEddsaMpcV2RecoveryTx into shared utils - #9449

Open
vibhavgo wants to merge 4 commits into
masterfrom
feat/sdk-core/extract-mpcv2-recovery-helper
Open

feat(sdk-core): extract isMpcV2Keycard and signEddsaMpcV2RecoveryTx into shared utils#9449
vibhavgo wants to merge 4 commits into
masterfrom
feat/sdk-core/extract-mpcv2-recovery-helper

Conversation

@vibhavgo

@vibhavgo vibhavgo commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Extracts shared EdDSA MPCv2 recovery helpers from duplicated coin implementations into sdk-core, then consumes them in abstract-substrate, sdk-coin-sol, and sdk-coin-ton.

New in sdk-core/eddsaMPCv2.ts:

  • EddsaSigningMaterial — discriminated union { version: 'v1'; userPrv } | { version: 'v2'; encryptedUserKey }
  • getEddsaSigningMaterial(userKey, passphrase, bitgo?) — detects v1 (JSON keycard) vs v2 (CBOR keycard), single decrypt, returns typed union
  • parseEddsaMpcV1Material(decrypted) — internal helper that both isEddsaMpcV1SigningMaterial and getEddsaSigningMaterial share to avoid double-decrypting v1 keycards
  • signEddsaMpcV2RecoveryTx(params) — full MPCv2 recovery signing: decrypt key shares → validate commonKeyChain → MPS DSG

Changes per module:

  • abstract-substrate: inlines decryptKeychain, collapses two separate wrappers into single signSubstrateMpcV2Recovery (protected so sinon can stub it via instance property)
  • sdk-coin-sol: replaces local EDDSAUtils.isEddsaMpcV1SigningMaterial calls with getEddsaSigningMaterial, simplifies isMpcv2SigningMaterial
  • sdk-coin-ton: replaces local duplicate of the detection + signing flow with shared helpers, renames private method to getEddsaSigningMaterial for consistency

Key fix: v1 keycards previously triggered two decrypts (one in isEddsaMpcV1SigningMaterial, one to retrieve userPrv). Extracting parseEddsaMpcV1Material as a shared JSON-structure check eliminates the second decrypt.

Test plan

  • sdk-core: yarn build && yarn unit-test — covers getEddsaSigningMaterial (v1/v2/sjcl-fallback), isEddsaMpcV1SigningMaterial, and signEddsaMpcV2RecoveryTx
  • abstract-substrate: yarn build (no runtime test env available) — unit tests cover MPCv1 and MPCv2 happy paths and error path
  • sdk-coin-sol: yarn build && yarn unit-test
  • sdk-coin-ton: yarn build && yarn unit-test

Ticket: WCI-1276

🤖 Generated with Claude Code

@vibhavgo
vibhavgo requested review from a team as code owners August 7, 2026 09:08
@vibhavgo
vibhavgo marked this pull request as draft August 7, 2026 10:18
@vibhavgo
vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from 1340716 to 32bab87 Compare August 10, 2026 05:21
@vibhavgo
vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from 32bab87 to 37ba5b1 Compare August 10, 2026 05:34
@vibhavgo
vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from cba5249 to 541f899 Compare August 10, 2026 06:17
@vibhavgo
vibhavgo requested a review from Marzooqa August 10, 2026 07:16
@vibhavgo
vibhavgo marked this pull request as ready for review August 10, 2026 07:31
Marzooqa
Marzooqa reviewed Aug 10, 2026
View reviewed changes

@Marzooqa Marzooqa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice consolidation — moving isMpcV2Keycard/signEddsaMpcV2RecoveryTx into sdk-core is the right call since the logic is genuinely duplicated across substrate/sol/ton (and soon ada/sui/near/iota per WCI-1276).

Two follow-up cleanup spots in abstractSubstrateCoin.ts now that the shared helpers exist — left inline.

Comment thread modules/abstract-substrate/src/abstractSubstrateCoin.ts Outdated
Comment thread modules/abstract-substrate/src/abstractSubstrateCoin.ts Outdated
@rishikeshdadam136

rishikeshdadam136 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Will defer to marzooqa review

davidkaplanbitgo
davidkaplanbitgo reviewed Aug 10, 2026
View reviewed changes

@davidkaplanbitgo davidkaplanbitgo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear why a BTC review is needed. Don't have enough context

@vibhavgo
vibhavgo requested a review from Marzooqa August 10, 2026 14:45
Base automatically changed from feat/abstract-substrate/WCI-1239 to master August 10, 2026 15:01
@vibhavgo
vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch 3 times, most recently from 7c1c3cc to 070b63f Compare August 10, 2026 16:36
dpkjnr
dpkjnr reviewed Aug 10, 2026
View reviewed changes
Comment thread modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts Outdated
Comment thread modules/sdk-core/src/bitgo/utils/tss/eddsa/eddsaMPCv2.ts Outdated
Comment thread modules/sdk-coin-sol/src/sol.ts Outdated
Comment thread modules/sdk-coin-sol/src/sol.ts Outdated
Comment thread modules/abstract-substrate/test/unit/abstractSubstrateCoin.ts Outdated
Comment thread modules/sdk-core/test/unit/bitgo/utils/tss/eddsa/eddsaMPCv2.ts Outdated
Comment thread modules/sdk-coin-ton/src/ton.ts Outdated
@vibhavgo
vibhavgo requested a review from dpkjnr August 11, 2026 07:49
vibhavgo and others added 4 commits August 11, 2026 14:44
@vibhavgo @claude
feat(sdk-core): extract and consume shared EdDSA MPCv2 recovery helpers
5a7cffa 
Ticket: WCI-1276

Add EddsaSigningMaterial discriminated union, isMpcV2Keycard, and
signEddsaMpcV2RecoveryTx to eddsaMPCv2.ts. Replace duplicated inline
implementations in abstract-substrate and sdk-coin-sol with these
shared helpers. sjcl fallback retained for optional bitgo parameter.
Export all new symbols from sdk-core package root.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vibhavgo @claude
feat(sdk-coin-ton): consume shared EdDSA MPCv2 recovery helpers
1ef8976 
Ticket: WCI-1276

Replace inline isMpcV2Keycard and addRecoverySignature MPCv2 block in
sdk-coin-ton with shared helpers from sdk-core. Removes TonSigningMaterial
local type in favour of EddsaSigningMaterial. Drop-in — no behaviour
change, no test modifications.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vibhavgo @claude
refactor(abstract-substrate): collapse MPCv2 wrappers, inline decrypt…
fa8b193 
…Keychain

Ticket: WCI-1276

Address PR review comments:
- Remove decryptKeychain private method; inline decryptKeychainPrivateKey
  at its one remaining call site (MPCv1 backup-key path)
- Replace getEddsaMpcV2RecoveryKeyShares + signEddsaMpcV2Recovery split
  wrappers with a single signSubstrateMpcV2Recovery wrapper that delegates
  to signEddsaMpcV2RecoveryTx from sdk-core
- Remove EDDSAUtils import (no longer referenced)
- Update test stubs to target the new consolidated wrapper

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vibhavgo @claude
refactor(sdk-core): rename isMpcV2Keycard, fix double-decrypt, drop t…
ee6720d 
…autological test

Ticket: WCI-1276

Address dpkjnr review comments:
- Rename isMpcV2Keycard → getEddsaSigningMaterial (returns material, not bool)
- Extract parseEddsaMpcV1Material() so isEddsaMpcV1SigningMaterial and
  getEddsaSigningMaterial share the JSON structure check; both now decrypt
  exactly once — eliminates double-decrypt on v1 keycards
- sjcl fallback now works for v1 in getEddsaSigningMaterial (no longer
  requires bitgo for v1 path)
- Strengthen getEddsaSigningMaterial v1 test: assert userPrv content and
  that decrypt was called exactly once
- Drop tautological mismatch test in abstract-substrate (sdk-core covers it)
- Update all rename sites: abstract-substrate, sdk-coin-sol, sdk-coin-ton
  (including ton test stubs)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vibhavgo
vibhavgo force-pushed the feat/sdk-core/extract-mpcv2-recovery-helper branch from 633b8e7 to ee6720d Compare August 11, 2026 09:15
@vibhavgo
vibhavgo requested a review from a team August 11, 2026 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidkaplanbitgo davidkaplanbitgo davidkaplanbitgo left review comments
@Marzooqa Marzooqa Marzooqa approved these changes
@danielpeng1 danielpeng1 Awaiting requested review from danielpeng1 danielpeng1 is a code owner automatically assigned from BitGo/wallet-core
@rishikeshdadam136 rishikeshdadam136 Awaiting requested review from rishikeshdadam136 rishikeshdadam136 is a code owner automatically assigned from BitGo/wallet-core-india
@dpkjnr dpkjnr Awaiting requested review from dpkjnr

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vibhavgo @rishikeshdadam136 @dpkjnr @davidkaplanbitgo @Marzooqa